-
Notifications
You must be signed in to change notification settings - Fork 24.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added fix for app freeze issue when modal closes #32450
Conversation
@feedthejim has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
PR build artifact for 7808b8e is ready. |
Base commit: 88ebec9 |
Base commit: 88ebec9 |
PR build artifact for b9b1718 is ready. |
I have applied this patch and can confirm it solved our problem with freezing modals on react-native v0.66.1 |
We have discovered that the issue is not fully fixed with the changes requested in this PR. When we present a react-native modal on top of a view controller that is also modally presented, when dismissing the modal it dismisses the underlaying modal also. It seems that RCTModalHostView:dismissModalViewController is being called twice. We tried to add the The following patch seems to solve the issue: diff --git a/node_modules/react-native/React/Views/RCTModalHostView.m b/node_modules/react-native/React/Views/RCTModalHostView.m
index bea9e08..b3bdeb7 100644
--- a/node_modules/react-native/React/Views/RCTModalHostView.m
+++ b/node_modules/react-native/React/Views/RCTModalHostView.m
@@ -116,10 +116,12 @@ - (void)didUpdateReactSubviews
- (void)dismissModalViewController
{
- if (_isPresented) {
- [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
- _isPresented = NO;
- }
+ dispatch_async(dispatch_get_main_queue(), ^{
+ if (_isPresented) {
+ [_delegate dismissModalHostView:self withViewController:_modalViewController animated:[self hasAnimationType]];
+ _isPresented = NO;
+ }
+ });
}
- (void)didMoveToWindow |
@patrickkempff So you're trying to say, that we should move the if condition inside the callback for the queue, and it kind of makes sense as well. I will get this fixed, thanks for highlighting this mate. |
I have also applied this patch and it works well on react-native v0.66.0 ! Thanks for this PR @hurali97 ! This must be merged @feedthejim it's quite annoying to have modal that freezes the app 😏 |
Awesome! |
@hurali97, thanks for PR! @feedthejim, please, merge it, it's very annoying 🙏🏻 |
Working fine now. Thanks for the PR @hurali97 Please merge this into main repo!! |
It looks like there is another problem with the modal window. Scenario:
Ugly workaround: use |
And one more issue with this fix. Scenario:
|
Hi, This is strange. Because I have tried your both use cases, but doesn't seem to repro them. Can you try to show the code. |
Hi. Ok, I'll try to create a minimally reproducible example, but later, I'm busy right now, sorry. I will write about the result here or create a new issue. I have reproduced this on a real device, iPhone 13 Pro, if that helps. |
+1. This fixes Modal dismiss issues for iOS on react-native 0.67.0-rc3 |
I can confirm this fixes the most basic use case of closing a modal, but somehow I can still experience the problem of app freezing (or at least modals not working anymore, even after killing the app). Still haven't found the root cause, but I'm certain it has something to do with a ghost The flow that exposes the problem in the app I'm developing is doing an app restart (we use this to allow devs to change the backend environment). |
I had an issue where after my modal dismissed it would re-appear right away and screen interaction would be broken. The patch above seems to work to fix that issue! Just wanted to call out some warnings that Xcode is showing with the new code: Simply following the auto-fixes clears the warnings and the fix still works. Might want to consider making this change @hurali97! |
Closing this as it's now already fixed and working as expected :) So it's needless to have it open. |
@hurali97 are you able to share which PR fixed this issue? Thanks! |
@hurali97 I just confirmed that this issue is not fixed in v0.69.3 of React Native. I'm not sure this should have been closed. The file touched by this pull request has not been updated on |
Fix for Modal Freeze Issue on iOS. Ref thread: facebook#32450
Patch is working fine in React-native. 0.70.10 |
Summary
As per the current version of react-native i.e, 0.66.0 , there is an issue when we close the modal, the whole app freezes. I looked at the code in RCTModalHostView, and the method named dismissModalViewController, seemed to be at fault, because it wasn't dismissing the view controller from the main thread, if I am not wrong. So I called the dismissal method from the main queue and voila, the app don't freeze anymore.
Other members are also facing this issue, so I tried to create a PR to get this fix in the main repo.
Issue: #32329
Changelog
[iOS] [Fixed] - There's one change in the file React/Views/RCTModalHostView.m/dismissModalViewController
Test Plan
Tested on react-native 0.66.0 , app don't freeze anymore after closing the modal